Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full conversion #2

Open
wants to merge 46 commits into
base: main
Choose a base branch
from
Open

Full conversion #2

wants to merge 46 commits into from

Conversation

abbyxh
Copy link
Contributor

@abbyxh abbyxh commented Jul 30, 2024

Completed full conversion of .xml -> .gltf with the need for a config file to speed up the conversion process. This contains the changes from #1.

required=True, type=str, nargs='+')
parser.add_argument('-o', '--out', help='Converted file path',
default='nothing', type=str)
parser.add_argument('-v', '--visibility', help='Level of layers in detector',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parser.add_argument('-v', '--visibility', help='Level of layers in detector',
parser.add_argument('-v', '--visibility-level', help='Level of layers in detector to consider in conversion',

Just to make it easier to understand

Comment on lines 9 to 10
parser.add_argument('-o', '--out', help='Converted file path',
default='nothing', type=str)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parser.add_argument('-o', '--out', help='Converted file path',
default='nothing', type=str)
parser.add_argument('-o', '--out', help='Converted file path',
default='', type=str)

Unlikely in this case but people might want to use whatever magical string that you put here. You can then no longer differentiate between that magic string and an actual user value. It's easier to deal with an empty string here, because an empty output file doesn't make sense.


args = parser.parse_args()

convert(args.compact, args.out, args.visibility)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
convert(args.compact, args.out, args.visibility)
convert(args.compact, args.out, args.visibility_level)

To keep things working after the argparse changes

ROOT.gGeoManager.SetVisLevel(visibility)
ROOT.gGeoManager.SetVisOption(0)

if out_path == 'nothing':

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if out_path == 'nothing':
if not out_path:

to keep things working after the argparse changes

Comment on lines 34 to 42
counter = 0
slash_list = []
for cfile in compact_files:
for i in cfile:
counter += 1
if i == '/':
slash_list.append(counter)

last_slash = max(slash_list)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this into a function to declutter the conversion function. I am also not entirely sure I understand what the output will be yet if there are multiple compact files.

The end result would look something like

    # ... all the rest from above
    ROOT.gGeoManager.SetVisOption(0)

    out_path = determine_outpath(out_path, compact_files)  # to be implemented
    ROOT.gGeoManager.Export(out_path)

convert(args.compact, args.out, args.visibility)


def convert(compact_files, out_path, visibility):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the same function as in conversion_xml2root.py so we could also just import it from there instead of copying it over.

Abigail Harrison added 2 commits August 1, 2024 14:20
…file so that an auto-generated configuration file can be created.
…w a certain amount of layers in your detector
Copy link

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. I have left a few comments on the implementation of tree that should make it independent of any global state / variables and some explanation on why that is a good thing to aim for.

One general thing for the configfile_generator.py script is the following. I would put everything that is not yet inside of a function into a main function (similar to what you already do for the full_conversion.py). That way we can then simply do (with many gaps for you to fill ;) )

from configfile_generator import tree, post_process`

# ... the rest of the code that loads the geometry from DD4hep ...

if not args.config_file:  # or whatever the exact argument name is
     det_tree = tree(description) 
     config = post_process(det_tree)  # potentially with more arguments
      # write config file
      
# ... The rest of the code that does the root -> gltf conversion (calls node) ...

# start = theDetector.detector("OpenDataTracker")
start = theDetector.world()

def tree(detElement, depth):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def tree(detElement, depth):
def tree(detElement, maxDepth=5, depth=0):

Passing in maxDepth as argument makes it possible to have this function being fully self contained and not depend on any other global / external values (like args.maxDepth at the moment). This makes it possible to import this function from another python module without any weird surprises because the value of that global variable might depend on whatever path is taken during imports.

Giving depth a default value of 0 makes it possible to simply call this function without it. Without this default value all users that call this will have to know that they need to pass in 0 here in order to get to their desired output.

depth += 1
children = detElement.children()
for raw_name, child in children:
if depth > args.maxDepth:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if depth > args.maxDepth:
if depth > maxDepth:

To use the argument that we pass rather than some global variable.

depth += 1
children = detElement.children()
for raw_name, child in children:
if depth > args.maxDepth:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if depth > args.maxDepth:
if depth > maxDepth:

To use the argument that we pass rather than some global variable.

if depth > args.maxDepth:
tree(child, depth)
else:
dictionary = tree(child, depth)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dictionary = tree(child, depth)
dictionary = tree(child, maxDepth, depth)

Now we also need to pass this in the recursion, obviously.

print('yay')


detector_dict = tree(start, 0)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
detector_dict = tree(start, 0)
detector_dict = tree(start, maxDepth=args.maxDepth)

And this becomes the place where we "connect" the value that we got from parsing the command line arguments with the value we use in the function.

Abigail Harrison added 2 commits August 2, 2024 18:40
…to the orrect form for the json file and created an output for automatic json file to be created so that this can be added into the complete .xml to .gltf conversion as an automatic config file. Also made some edits to silly mistakes in the 'tree' function.
… to the full conversion of xml to gltf. Have started the full conversion included the automatic config file however dd4hep and ROOT python imports seem unable to work together in the same python script so I am currently thinking of using python subprocess to still run the conversion together.
Copy link

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good already. We will have to do some reorganizing and refactoring to remove some of the duplicated code, but we will do that once I am back. I would move some of the files around:

  • All the python scripts should go into bin
  • If you have auto-generated json config files, I would put them into a new configs/examples folder, where you can also put a README.md file that can hold 1-2 sentences on how they were obtained.

def post_processing(obj, main_parts, subParts={}, sublist= []):
for k, v in obj.items():
if k in main_parts:
sublist = [f'{k}_(?!envelope)\\w+']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the people who don't know regex by heart (like me). Could you put a comment here briefly explaining what (!?envelope) does in the generated regular expression?

@@ -0,0 +1 @@
{"childrenToHide": [], "subParts": {"BeamCal": [["BeamCal_(?!envelope)\\w+"], 0.8], "Coil": [["Coil_(?!envelope)\\w+"], 0.8], "EcalBarrel": [["EcalBarrel_(?!envelope)\\w+"], 0.8], "EcalEndcap": [["EcalEndcap_(?!envelope)\\w+"], 0.8], "EcalPlug": [["EcalPlug_(?!envelope)\\w+"], 0.8], "FTD": [["FTD_(?!envelope)\\w+", "FTDDisk_.*_negZ", "ftd_petal_negZ_.*_.*", "ftd_sensor_negZ_.*_.*_.*", "FTDDisk_.*_posZ", "ftd_petal_posZ_.*_.*", "ftd_sensor_posZ_.*_.*_.*", "FTD_support"], 0.8], "HcalBarrel": [["HcalBarrel_(?!envelope)\\w+"], 0.8], "HcalEndcap": [["HcalEndcap_(?!envelope)\\w+"], 0.8], "HcalRing": [["HcalRing_(?!envelope)\\w+"], 0.8], "LHCal": [["LHCal_(?!envelope)\\w+"], 0.8], "Lcal": [["Lcal_(?!envelope)\\w+"], 0.8], "QD0_cryostat": [["QD0_cryostat_(?!envelope)\\w+"], 0.8], "QD0_support": [["QD0_support_(?!envelope)\\w+"], 0.8], "SET": [["SET_(?!envelope)\\w+", "set_ladder_.*_.*", "set_ladder_.*_.*_.*"], 0.8], "SIT": [["SIT_(?!envelope)\\w+", "sit_ladder_.*_.*", "sit_ladder_.*_.*_.*"], 0.8], "SServices00": [["SServices00_(?!envelope)\\w+"], 0.8], "TPC": [["TPC_(?!envelope)\\w+", "tpc_endcap_bwd", "tpc_endcap_fwd", "tpc_sensGas_bwd", "tpc_row_bwd_.*", "tpc_sensGas_fwd", "tpc_row_fwd_.*"], 0.8], "Tube": [["Tube_(?!envelope)\\w+"], 0.8], "VXD": [["VXD_(?!envelope)\\w+", "BerylliumAnnulusBlock_.*_negZ_.*", "BerylliumAnnulusBlock_.*_posZ_.*", "ElectronicsEnd_.*_negZ_.*", "ElectronicsEnd_.*_posZ_.*", "SiActiveLayer_.*_negZ_.*", "SiActiveLayer_.*_posZ_.*", "VXD_support", "KaptonLines_.*_negZ", "KaptonLines_.*_posZ", "MetalLines_.*_negZ", "MetalLines_.*_posZ", "VXD_ExtKaptonCab_neg", "VXD_ExtKaptonCab_pos", "VXD_ExtMetalCab_neg", "VXD_ExtMetalCab_pos", "VXD_endplate_bwd", "VXD_endplate_fwd", "VXD_support_bwd_inner", "VXD_support_bwd_outer", "VXD_support_fwd_inner", "VXD_support_fwd_outer", "negShellCone", "posShellCone"], 0.8], "YokeBarrel": [["YokeBarrel_(?!envelope)\\w+"], 0.8], "YokeEndcap": [["YokeEndcap_(?!envelope)\\w+"], 0.8]}, "maxLevel": 3}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto-generated output? Very nice. If you want to make it more readable you can use the indent parameter of the json.dump function.

detector_dict = tree(start, 0, args.maxDepth)
#pprint.pprint(detector_dict)

def post_processing(obj, main_parts, subParts={}, sublist= []):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this also be made to take a list of subParts to ignore? That would make it possible to define a list that can be put into childrenToHide without leading to conflicts

Abigail Harrison added 3 commits August 8, 2024 08:58
…e geometries and provide an automatic configuration file. I have made a new function 'determine_outpath' to give an automatic path for the files if they are not given.
… to be added to. Tidied up the code a bit to make it more usable and readable
… running of the script so that they can delete the root file that has been created once they have the gltf file and they can also stop the python script early to modify the config_file. I then also responded to some of the comments so everything has been checked and cleaned up a bit.
@abbyxh
Copy link
Contributor Author

abbyxh commented Aug 8, 2024

@tmadlener Hi to respond to a few of your comments: I have changed most of the suggestions you have made. I realise I was late to putting in a few of them so sorry about that :) . The suggestions I haven't added in yet includes adding the childrenToHide subparts. Should I have this as something the user can input and say for example I don't want to see this part of the detector? And also I wasn't able to import the configfile_generator.py to call on it with the conversion script as there seems to be a clash with dd4hep and ROOT in the C++ files. So I'm using subprocess which is working well. If you have anymore comments on what I've written let me know :)

Abigail Harrison added 2 commits August 9, 2024 15:45
…). Also trying to add in a way to add hidden-children into the automatic config file creation and maybe find a way for multiple files to be inputed at one time.
…r that they want to have removed to 'childrenToHide' so that they are not displayed on the gltf file
Abigail Harrison added 7 commits August 12, 2024 17:16
…ng instructions in the README.md of how the python scripts can be used and what parameters they take in
…reg ex wouldn't check for capital letters as some parts of the detector have stopped getting called due to a change of capitalisation.
…nd to set the ILD default colours for the Phoenix event display. Not finished and very messy but I've set what each subpart of the detector colour should be I just need to add in the colours and then see if it works. This is all done in phoenixExport.js
… colours they want for each subdetector into the config file and it will apply it to the detector. So far, the automatic config file generator doesn't add colours so that needs to be added and there needs to be an option for the default colours to be used if the user doesn't input any colours.
…g isn't provided by the user for the subdetector, the colour of the subdetector is set to the original automatic colouring so that its easy for the user to change certain colours if they want to without needing to do every subdetector
…g file that is produced so that it doesn't need to be changed too much when editing the config file
README.md Outdated

The script can also be run by inputing a configuration file, a root file or both. The converter automatically saves the files root, gltf and config files in automatic folders named:

* `root_files` -root files can choose to be delted at the end of the run if the user wishes to

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `root_files` -root files can choose to be delted at the end of the run if the user wishes to
* `root_files` -root files can choose to be deleted at the end of the run if the user wishes to

README.md Outdated

To run the python script in the bin folder with only a compact file:


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```bash


The user can input where they'd like to save the files:

python conversion_xml2gltf_autoConfig.py -cm <your-detector-file.xml>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
python conversion_xml2gltf_autoConfig.py -cm <your-detector-file.xml>
```bash
python conversion_xml2gltf_autoConfig.py -cm <your-detector-file.xml>


The user can also define how many layers of the detector they would like to see and if they would like to hide any parts of the detector:


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```bash

for s in subdetector_dict:
find_subdetector = re.search(f'{s}', f'{subdetector}',re.IGNORECASE)
if find_subdetector: return subdetector_dict[s]
if find_subdetector == None: return [0.57,0.63,0.81]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if find_subdetector == None: return [0.57,0.63,0.81]
return [0.57,0.63,0.81]

This if condition is not necessary here, because in any case where you find a suitable subdetector, you return before this line is reached. So if we come here, we can simply return the default values here.

Where do the defaults come from?

Comment on lines 75 to 76
if depth > maxDepth: tree(child, depth, maxDepth)
else: dictionary = tree(child, depth, maxDepth); nd.update({raw_name: dictionary})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if depth > maxDepth: tree(child, depth, maxDepth)
else: dictionary = tree(child, depth, maxDepth); nd.update({raw_name: dictionary})
if depth > maxDepth:
tree(child, depth, maxDepth)
else:
dictionary = tree(child, depth, maxDepth)
nd.update({raw_name: dictionary})

Makes it a bit easier to read.

outer_list.append(0.8)

## Adds automatic ILD coloring if the user asks
if coloring == "ild": color = add_ild_colors(k); outer_list.append(color)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if coloring == "ild": color = add_ild_colors(k); outer_list.append(color)
if coloring == "ild":
color = add_ild_colors(k)
outer_list.append(color)

Comment on lines 154 to 158

//for (var mat of gltf["materials"]) {
//console.log(mat);
//}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//for (var mat of gltf["materials"]) {
//console.log(mat);
//}

(assuming this was mainly for debugging)

@@ -66,10 +66,69 @@ function cleanupGeometry(node,
}
}

//function to generate alternative colouring that the user can set within the config file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a brief explanation in English / bullet points on what this function does on a high level, i.e. don't describe what it does by following the code, but rather what its effects are in the end, and potentially also why things are done this way.

IIUC: we are assigning colors to the materials that we find in the gltf (resp. to some property of that)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants